-
Notifications
You must be signed in to change notification settings - Fork 398
feat: comparison ops for list and struct types #6104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Greptile OverviewGreptile SummaryThis PR implements comparison operations ( Key Changes:
Minor Issue:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Series
participant ListArray
participant RowFastPath
participant ElementWise
participant RowConverter
User->>Series: compare(lhs, rhs, op)
Series->>Series: Check if List/FixedSizeList/Struct type
alt List/FixedSizeList/Struct
Series->>ListArray: compare_list_like_array(lhs, rhs, op)
ListArray->>ListArray: comparison_broadcast_info()
ListArray->>RowFastPath: row_fast_path()?
alt No nulls + No floats + Supported type
RowFastPath->>RowFastPath: type_contains_float() → false
RowFastPath->>RowFastPath: array_has_any_nulls() → false
RowFastPath->>RowConverter: convert_columns()
RowConverter-->>RowFastPath: row representations
RowFastPath->>RowFastPath: Compare rows using cmp()
RowFastPath-->>ListArray: Some(BooleanArray)
ListArray-->>Series: BooleanArray
else Contains nulls/floats or unsupported
RowFastPath-->>ListArray: None (fallback)
ListArray->>ElementWise: compare_list_like_value() per element
loop For each array index
ElementWise->>ElementWise: Get element at index
ElementWise->>Series: compare_scalar_series()
Series-->>ElementWise: Option<bool>
end
ElementWise-->>ListArray: BooleanArray
ListArray-->>Series: BooleanArray
end
else Primitive types
Series->>Series: Use existing comparison logic
end
Series-->>User: BooleanArray result
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 files reviewed, 1 comment
| pub fn is_comparison(&self) -> bool { | ||
| matches!( | ||
| self, | ||
| Self::Eq | ||
| | Self::EqNullSafe | ||
| | Self::NotEq | ||
| | Self::Lt | ||
| | Self::LtEq | ||
| | Self::Gt | ||
| | Self::GtEq | ||
| ) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check that is_comparison() behavior change doesn't break existing code. Old version included logical operators (And/Or/Xor), new version correctly excludes them
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/daft-core/src/operator.rs
Line: 48:59
Comment:
Check that `is_comparison()` behavior change doesn't break existing code. Old version included logical operators (`And`/`Or`/`Xor`), new version correctly excludes them
How can I resolve this? If you propose a fix, please make it concise.ca92074 to
d694e63
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6104 +/- ##
==========================================
- Coverage 43.34% 43.26% -0.09%
==========================================
Files 917 921 +4
Lines 112995 114222 +1227
==========================================
+ Hits 48978 49417 +439
- Misses 64017 64805 +788
🚀 New features to boost your workflow:
|
d694e63 to
4cf4b23
Compare
| } | ||
|
|
||
| #[test] | ||
| fn compare_list_arrays_basic() -> DaftResult<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it'd be nice to have a few meor test cases here. maybe we could use rstest to create some parametrized tests to cover a larger area. We have other parts of the codebse that use parametrized rstests.
universalmind303
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks great! good work @aaron-ang
two things i'd like to see before we merge this in.
- a few more rust based unit tests
- python unit tests testing the compare ops using series & dataframes.
Changes Made
Operatorenum todaft-coreto reuse comparison ops.arrow-row'sRowConverterif specific conditions are met, falling back to element-wise comparison.Related Issues
Closes #3510.